-
Notifications
You must be signed in to change notification settings - Fork 4.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Attempt to fix DuplicateAndClose_TcpServerHandler flaky test #103161
Conversation
Tagging subscribers to this area: @dotnet/ncl |
Related failure log snapshot:
|
src/libraries/System.Net.Sockets/tests/FunctionalTests/SocketDuplicationTests.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/azp run runtime-libraries-coreclr outerloop |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run runtime-libraries-coreclr outerloop |
Azure Pipelines successfully started running 1 pipeline(s). |
src/libraries/System.Net.Sockets/tests/FunctionalTests/SocketDuplicationTests.cs
Show resolved
Hide resolved
/azp run |
You have several pipelines (over 10) configured to build pull requests in this repository. Specify which pipelines you would like to run by using /azp run [pipelines] command. You can specify multiple pipelines using a comma separated list. |
/ba-g After careful review I am quite sure failing tests are not related to changes of this PR |
Context:
We have noticed failure of DuplicateAndClose_TcpServerHandler test in Job https://dev.azure.com/dnceng-public/public/_build/results?buildId=696224&view=logs&j=cb76e204-ba8f-557d-eba6-9896b4f24865
Changes made (changes are isolated into commits for easier review/revert):
Ensure RemoteInvokeHandle is disposed - this is ensuring that Dispose of
RemoteInvokeHandle
will be called during test run, otherwise it would be called in Finalize and as it throws, it would shutdown whole xunit runner.Make RunCommonHostLogic synchronous - after discussion with original author we decide reverting
RunCommonHostLogic
back to synchronous execution for simplicity reasons.Document RemoteInvokeHandle DisposeAsync intent.
Replace namedpipe IPC by Socket IPC - as there is theory that namedpipe implementatation is root cause of this flakiness, I have decided to rule it our by replacing it by simple
Socket
based IPC. IPC is required because remote code has to start before socket is disconnect, as socket disconnection requires process ID of the other process.Testing:
locally by unit tests